azurerm_batch_pool - Support for Container Registry configuration #4072
Conversation
- Add documentation
- Use hashed ID for attributes validation in tests - Expect non-empty plan for tests dealing with container registry changes
There was a problem hiding this comment.
Hi @ennui93,
Thank you for the PR. My main concern is that we use actual schema instead of a TypeMap for all the properties of the container registry.:
"container_registries": {
Type: schema.TypeSet,
Optional: true,
computed: true,
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"registry_server": {
...
},
"user_name": {
...
},
"password": {
...
},
},
},
},
- Use list of Resources rather than generic set for container registries
|
Hi @katbyte, thank you for your review. I have revamped the PR with the schema as requested. Please review and provide any pertinent feedback at your convenience. |
There was a problem hiding this comment.
Thanks for the revisions @ennui93,
In addition to my comments inline could we update the datasource documentation?
My main concern is that we are reading the probably blank password back into state & expecting a non empty plan. What we should do is pass along the password thats already in the state.
- Find and save to state container registry password as supplied by user - Documentation: Make explicit where new resources must be created - Documentation: Format alignment - Validate container registry parameters are not empty - Update integration tests per new password behavior as in first point
|
Hi @katbyte, thanks again for your second review. I believe I have addressed all of your comments. You are correct that I was unaware how to pass the password supplied by the user into the state. The solution looks much better now. Please review at your convenience. |
katbyte
changed the title
|
This has been released in version 1.33.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example: provider "azurerm" {
version = "~> 1.33.0"
}
# ... other configuration ... |
|
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks! |
ghost
locked and limited conversation to collaborators
Add a
container_registriesblock to the batch pool container configuration resource.This setting is necessary for Azure Batch service Pools to support custom container images (i.e. from private container repositories).